-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update GPIO API to support flags used by Linux DT bindings #16648
Conversation
Found the following issues, please fix and resubmit: checkpatch issues
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This review is for the API changes only. I didn't have time time to look at the samples or driver changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates! I've had some time to look at the use in drivers and applications and have some questions.
Keep up the great work!
@@ -91,18 +93,31 @@ static int gpio_gecko_configure(struct device *dev, | |||
return -ENOTSUP; | |||
} | |||
|
|||
if ((flags & GPIO_DIR_MASK) == GPIO_DIR_IN) { | |||
if ((flags & GPIO_PUD_MASK) == GPIO_PUD_PULL_UP) { | |||
if (flags & GPIO_OUTPUT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I was looking at the flags, I wondered what the code would do when both GPIO_OUTPUT and GPIO_INPUT were set.
This driver respects output before input, but I don't see it documented that OUTPUT always has higher priority if both are set. Is that your intent?
I think it should be documented if so. Though my preference would be slightly for rejecting this as an invalid combination -- perhaps a common helper function, gpio_flags_are_valid(u32_t flags)
, could be added for this purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several devices where it's perfectly valid and very useful to have a particular GPIO configured for input and output simultaneously. The API should allow that, on those devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. What are the intended semantics if the user requests both but the driver only supports one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dunno; -ENOTSUP
would seem to make sense, though -EINVAL
is used here for a combination that would be acceptable on other platforms (GPIO_INT
with GPIO_OUTPUT
should be fine if GPIO_INPUT
is also set).
In this case the comments suggest that bidirectional is handled in GPIO_OUTPUT
case, though I don't see how that works and it's not how I would expect it to be expressed for Nordic or SX1509B. In those input and output are simply independent functions.
The API documentation needs to be reviewed and updated to describe exactly what is and is not allowed, and what behavior the different combinations have to provide in order to meet the API's promises. GPIO_DEBOUNCE
is one I noticed that still has about the same chance for misinterpretation as GPIO_POL_INV
did in the current API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-ENOTSUP
sounds like a good plan to me for unsupported (but valid) combinations of flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit of code was written specifically for the gecko GPIO module. In case any of the output modes are enabled the input will also be implicitly enabled. This is actually a fairly common approach among SoC vendors but surely not the only one. Application code that wants to read from the input must pass GPIO_INPUT
flag to the configure function. In case of the gecko platform if it passes GPIO_OUTPUT
only the input direction will still work. A portable application code shouldn't rely on this 'feature'. The gecko GPIO driver doesn't perform extra checks, e.g. the implementation of gpio_pin_get
function doesn't check if the input direction was explicitly configured as this would increase function's code size, make it slower.
As for the GPIO_OUTPUT
, GPIO_INPUT
flag usage. If an application wants to write to the pin it should pass GPIO_OUTPUT
flag to the configure function, if it intends to read from the pin it should pass GPIO_INPUT
flag. If it intends to write and read it should pass both flags. If no flag is passed the pin will be disabled.
In general, if the function is not able to provide features requested by the configuration flags it should return -ENOTSUP
.
I agree though we should better document error code / assert usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree though we should better document error code / assert usage.
That would be great if you don't mind. I really like your proposal of adding asserts into gpio.h itself in addition to docs. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requesting changes while semantic specification debate is ongoing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to have a single function to configure GPIO interrupts and GPIO I/O characteristics is a misconception. There is a reason why Linux only provides GPIO DT bindings for the latter.
In my use case I have a SPI chip select handled by hardware (pin muxed to SPI) where I still want to get interrupts on an active chip select. If I would configure the interrupt by gpio_pin_configure() the pin would be muxed to the GPIO function.
Interrupt configuration should be done in the gpio_callback struct. (E.g. in STM32 interrupt generation and GPIO are even two distinct blocks.)
Without deep analysis this makes a lot of sense. Some drivers would have to make sure they preserve non-interrupt-related configuration as interrupts are enabled and disabled, but that shouldn't be too painful. Depending on the model used to describe interrupt vs gpio configurations I can imagine that for some hardware the GPIO might have to be changed during the periods when interrupts are enabled (e.g. enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
My little doubt is only regarding flags with value of 0, like GPIO_INT_LEVELS_PHYSICAL
or GPIO_INT_LEVEL
.
Without checking the actual value of such flag, perhaps when modifying some code, one could try to use something like if (flags & GPIO_INT_LEVELS_PHYSICAL)
, which obviously won't work as expected. We need to support the flags of value 0 defined in DT bindings, but maybe we could get rid of these two (not add GPIO_INT_LEVELS_PHYSICAL
and deprecate GPIO_INT_LEVEL
)? Are they actually useful?
And I support the @b0661's proposal of moving the interrupt configuration flags to the callback structure. This would also allow creating generic parts of code that could enable/disable/reconfigure interrupts without having to know the actual pin configuration.
3eaf26b
to
cf88266
Compare
That's indeed a limitation of this approach. When writing a device driver one needs to always check the definition of the flag not to use a 'dummy' one. They shouldn't be used by the device drivers, only by the application level code. And in this case they make the code easier to read. The application level code will never want to perform any operation on these flags so there is no danger that the flag will be misused. It can be misused by the author of the device driver but this is entirely under our control, happens only once when writing the driver and should be caught when testing the code. I removed |
All checks are passing now. Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
static inline int gpio_pin_set(struct device *port, unsigned int pin, int value) | ||
{ | ||
struct gpio_driver_data *const data = | ||
(struct gpio_driver_data *const)port->driver_data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're making an immutable pointer to mutable data, which isn't very useful and looks weird because other pointers aren't treated that way. The fixup I provided made clear that the code should not be changing invert
:
- struct gpio_driver_data *const data = port->driver_data;
+ const struct gpio_driver_data *data =
+ (const struct gpio_driver_data *)port->driver_data;
3 occurrences if you want to change it.
This commit makes following changes to GPIO dt-bindings flags: - Added GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH to indicate pin active state. - Added GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE to configure single ended pin driving mode. - Added GPIO_PULL_UP, GPIO_PULL_DOWN flags. - GPIO_INPUT, GPIO_OUTPUT to configure pin as input or output. - Added GPIO_OUTPUT_LOW, GPIO_OUTPUT_HIGH flags to initialize output in low or high state. - reworked GPIO_INT_* flags to configure pin interrupts. - following flags were deprecated: GPIO_DIR_*, GPIO_DS_DISCONNECT_*, GPIO_PUD_*, GPIO_INT_ACTIVE_*, GPIO_INT_DOUBLE_EDGE, GPIO_POL_*. To be aligned with Linux DTS standard any GPIO flags that should not be used in DTS files are moved from include/dt-bindings/gpio/gpio.h file to include/drivers/gpio.h with an exception of several old flags which removal would cause DTS compilation errors. Those remaining old flags will be removed from include/dt-bindings/gpio/gpio.h at a later stage. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
This commit adds following functions which work with pin logical levels (take into account GPIO_ACTIVE_LOW flag): - gpio_port_get, gpio_port_set_masked, gpio_port_set_bits, gpio_port_clear_bits, gpio_port_set_clr_bits - gpio_pin_get, gpio_pin_set Functions which work with pin physical levels: - gpio_port_get_raw, gpio_port_set_masked_raw, gpio_port_set_bits_raw, gpio_port_clear_bits_raw, gpio_port_set_clr_bits_raw - gpio_pin_get_raw, gpio_pin_set_raw As well as functions: - gpio_port_toggle_bits, gpio_pin_toggle_bits Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
This commit moves interrupt configuration for a single pin from gpio_pin_configure to gpio_pin_interrupt_configure function. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
Both pin sets and values encoding pin values are ultimately represented by 32-bit unsigned integers. Provide typedefs that make the role of a parameter explicit. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no> Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
gpio_api_1pin testcase verifies following new GPIO API functionality: - pin active level flags - input/output configuration flags - pin drive flags - gpio_port_*, gpio_pin_* functions - pin interrupts The test is using only 1 GPIO pin (defined in DTS as LED0) and relies on the ability of the driver to configure pin simultanously as in/out. Drivers that do not allow to configure pins in in/out mode should still pass the test, however, most of the testcases will be skipped. The test does not require any modifications to board hardware. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API as well as gpio_pin_interrupt_configure function. Tested on efr32_slwstk6061a board. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
b7eeb42
to
1933e3e
Compare
Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API as well as gpio_pin_interrupt_configure function. Tested on sam_e70_xplained board. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
1933e3e
to
ef3624e
Compare
Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API as well as gpio_pin_interrupt_configure function. Tested on nrf52840_pca10056 board. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com> Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API as well as gpio_pin_interrupt_configure function. Tested on frdm_k64f board. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
Update gpio_pin_configure() to take into account GPIO flags defined by the devicetree. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
Update gpio_pin_configure() to take into account GPIO flags defined by the devicetree. Use gpio_pin_get/gpio_pin_set to verify reading/writing of logical pin values. Use gpio_pin_interrupt_configure() to configure interrupts. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
Temp patch to see if this at least passes a full shippable build or not. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
ef3624e
to
db88003
Compare
Pulled in dc2d4c0 and merged with the NRF commit. |
This is a first PR in series which updates GPIO API, drivers and samples to implement changes discussed in #15611, #10339. It is based on work done in #12651, #11880.
The primary focus of this PR is to introduce standard flags used by Linux DT bindings. The specific changes implemented:
driving mode.
Support for GPIO_PERSISTENT, GPIO_TRANSITORY flags (bit 3) will be added later.
Remaining changes:
Added flags to support gpio and pinctrl bindings defined in form of properties.
All GPIO flags which should not be used in DTS were removed from DT bindings gpio.h file. Support for DTS properties of GPIO node such as: input, output-low, output-high, line-name, etc. will be added in the future.
GPIO_INT_* flags were reworked to support configuration of pin interrupts on logical or physical levels.
Following flags were deprecated: GPIO_DIR_, GPIO_DS_DISCONNECT_, GPIO_PUD_, GPIO_INT_ACTIVE_, GPIO_INT_DOUBLE_EDGE, GPIO_POL_*.
Added gpio_pin_set, gpio_pin_get functions which work with pin logical levels, taking into account GPIO_ACTIVE_LOW flag
gpio_sam, gpio_gecko drivers along with .dts and other infrastructure files were updated to reflect above changes. Also 'button' and 'blinky' samples were updated to use the new GPIO flags.
This PR is not yet complete. To be able to merge it we need to update all existing GPIO drivers, samples and tests to use the new flags.
Any functional changes not mentioned here will be addressed in subsequent PRs. Specifically this PR doesn't modify flags related to pin drive strength, debouncing neither attempts to introduce functions operating on ports.